Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

datastore: use transaction object #1369

Merged
merged 4 commits into from
Jun 23, 2016

Conversation

stephenplusplus
Copy link
Contributor

@stephenplusplus stephenplusplus commented Jun 8, 2016

Fixes #633

The goal of this change is to give more visibility into the operations being performed when using transactions.

Before

datastore.runInTransaction(function(transaction, done) {
  transaction.save({
    key: key,
    data: { value: true }
  });

  done();
}, function(err) {
  // Where is this error coming from?
});

After

var transaction = datastore.transaction();

transaction.run(function(err) {
  if (err) {
    // Error handling omitted.
  }

  transaction.save({
    key: key,
    data: { value: true }
  });

  transaction.commit(function(err) {
    if (!err) {
      // Transaction committed successfully.
    }
  });
});

The diff scariness doesn't reflect reality... I organized method names alphabetically. The code bodies themselves didn't change, just their location in the file & doc blocks.

@pcostell is this a good change in terms of usability?

@stephenplusplus stephenplusplus added the api: datastore Issues related to the Datastore API. label Jun 8, 2016
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jun 8, 2016
@pcostell
Copy link
Contributor

What happens if there is an error? Do we rollback automatically? Does the user have to do this manually?

@stephenplusplus
Copy link
Contributor Author

Nope, and I'll add a to do item to make sure we rollback if the commit call fails. So, how does this case work:

transaction.commit(function(err) {
  // Behind the scenes, we see that this call fails.
  // So before executing this callback, we should automatically rollback.

  // What happens if the user didn't catch that part in the docs, and tries
  // to rollback themselves?
  if (err) {
    transaction.rollback(...);
  }
});

@pcostell
Copy link
Contributor

We just need to make sure to swallow any errors on the automatic rollback -- it will get an error:

INVALID_ARGUMENT: The referenced transaction has expired or is no longer valid.

@pcostell
Copy link
Contributor

/cc @jdimond

@stephenplusplus
Copy link
Contributor Author

Added a commit to automatically rollback after a failed commit: 41eec47

// Rollback automatically for the user.
self.rollback(function(err) {
if (err) {
err.message = [

This comment was marked as spam.

This comment was marked as spam.

@pcostell
Copy link
Contributor

To verify -- if an error happens during the BeginTransaction call, all attempted calls as part of that transaction will fail, correct?

@pcostell
Copy link
Contributor

Are we worried users may try to do something like:

datastore.transaction().run(....) and then use datastore instead of transaction inside that run method?

@stephenplusplus
Copy link
Contributor Author

if an error happens during the BeginTransaction call, all attempted calls as part of that transaction will fail, correct?

var transaction = datastore.transaction();

transaction.run(function(err) {
  if (err) {
    // BeginTransaction failed
  }
});

Calling anything else from transaction will return whatever error the API will.

Are we worried users may try to do something like: datastore.transaction().run(....) and then use datastore instead of transaction inside that run method?

That's a fair concern. We could return the transaction as the second argument to the run callback:

transaction.run(function(err, tx) { /* transaction === tx */ });
datastore.transaction().run(function(err, transaction) {});

@jgeewax
Copy link
Contributor

jgeewax commented Jun 14, 2016

Just to be sure -- there are cases where someone might want to start a transaction, and then from outside the transaction execute a query of sorts, right?

If there is no good reason to ever do:

transaction.run( (err) => {
  datastore.run(somethingElse);
  transaction.commit();
});

then we should throw an error if people try to do something like that.... ?

@stephenplusplus
Copy link
Contributor Author

I think I'm a bit confused by your example. run is only a method on a Transaction object, so
datastore.run(somethingElse); would throw.

@jgeewax
Copy link
Contributor

jgeewax commented Jun 14, 2016

Sorry - I mean it as an example of "running another query from inside that block". Is there a time when that would make sense? That'd I'd get an entity while inside a transaction block, but not want to use the transaction ?

@stephenplusplus
Copy link
Contributor Author

Gotcha. In terms of a use case, I'm not sure.

I don't think I'd block those operations, though. I'm not sure how, actually... choosing where a function can be executed or not:

transaction.run(function (err) {
  datastore.get(key, function(err, entity) {})
  //        ^-- need to detect this was executed inside of `transaction.run`
})

If this is what we want, maybe @callmehiphop and I can think of something clever. However, I think the user would expect if they have something that works in one place, it would work wherever they choose to put it.

@stephenplusplus
Copy link
Contributor Author

@callmehiphop PTAL

@coveralls
Copy link

coveralls commented Jun 23, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling 3f8c632 on stephenplusplus:spp--633 into 50d465b on GoogleCloudPlatform:master.

@coveralls
Copy link

coveralls commented Jun 23, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling 5aa2c44 on stephenplusplus:spp--633 into 50d465b on GoogleCloudPlatform:master.

@callmehiphop callmehiphop merged commit 535b188 into googleapis:master Jun 23, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: datastore Issues related to the Datastore API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants